Fix caller transaction graph handoff - #367
Conversation
There was a problem hiding this comment.
Review verdict: changes requested
Three independent specialist passes reviewed this head for PostgreSQL transaction semantics, durable replay compatibility, and Rust reliability. The transaction-handoff direction is sound, but three production correctness/liveness issues and two regression-test gaps should be resolved before merge. CI remains the source of test results.
cdbc456 to
f479654
Compare
|
@microsoft-github-policy-service agree |
f479654 to
80d3df0
Compare
|
Rebased this branch onto current \main\ (\3a8dbb, #368) to resolve the \CHANGELOG.md\ merge conflict that appeared after the v0.2.7 release commit landed. Combined both edits: kept upstream's dated header/PR-number annotations and the new Changed section, and re-inserted the Caller-transaction handoff bullet. No other files touched by the rebase. New head: 80d3df0. Re-ran the full validation suite (fmt, clippy -D warnings on PG17+PG18, pgrx unit tests 309/0/16 on both majors, full E2E 56/0 on both majors including 68_long_caller_transaction, upgrade tests 86/86, plus 3 additional repeat runs of 68_long_caller_transaction on each major) against this rebased tree -- all green. |
Track the originating PostgreSQL transaction while the worker waits for a newly started graph. Use durable, bounded probes so long commits resume, rollbacks fail cleanly, transient database errors retry, and wait history remains bounded. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Resolves all 5 line comments from the automated review of the caller-mode df.start() transaction-handoff fix: 1. HIGH: snapshot-visibility race after pg_xact_status() reports "committed". Postgres can mark a transaction committed before ProcArrayEndTransaction removes it from the running set used by a fresh snapshot, so an immediate re-read could misclassify a valid graph as CommittedMissing. Now checks pg_visible_in_snapshot(origin_xid, pg_current_snapshot()) first and retries until the xid is snapshot-visible before trusting a re-read. 2. HIGH: fixed 2s deadline could livelock a valid graph load (role validation + up to MAX_GRAPH_NODES rows + serialization). Graph loading now uses its own GRAPH_LOAD_QUERY_TIMEOUT (20s client-side) plus Postgres-side statement_timeout/lock_timeout backstops, decoupled from the short TRANSACTION_PROBE_QUERY_TIMEOUT used for cheap visibility and transaction-status probes. 3. HIGH: permanent database errors retried forever. Added SQLSTATE classification (classify_sqlstate/classify_sqlx_error): an allowlist of genuinely transient codes (connection failures, serialization/deadlock, our own statement/lock timeouts, admin-cancel/recovery conflicts) retries as before; everything else (insufficient_privilege, undefined_table, undefined_function, schema/decode errors, ...) now fails the activity immediately with its SQLSTATE embedded. Also added a bounded transient retry budget (MAX_GRAPH_RETRY_ATTEMPTS) tracked independently from the caller-transaction wait counter, so transient DB failures can no longer poll indefinitely even when individually "retryable". 4. MEDIUM: the regression test didn't prove the timeout-retry path executed and could pick an unrelated backend by pid alone. Rewrote 68_long_caller_transaction.sql to identify the graph-probe backend by its exact query text, and to require a strictly later query_start (rather than a different pid, since the management pool may legitimately reuse a connection) for both the post-kill retry and a further, independent timeout-driven retry before the lock is released. Also fixes a real bug found while validating this test: pg_stat_activity's snapshot is cached for the lifetime of the enclosing transaction, so a long-lived DO block must call pg_stat_clear_snapshot() before every poll to observe live backend state. 5. LOW: false OR NULL evaluates to NULL in PL/pgSQL, so a NULL engine_output on failure bypassed the rollback assertions. Added engine_output IS NULL OR ... to both rollback checks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Move the caller-transaction handoff changelog entry to the 0.2.8 Unreleased section after rebasing onto origin/main. - Apply server-side statement_timeout/lock_timeout to every cheap probe (visibility, transaction-status, snapshot) and the role superuser check so a blocked probe is cancelled by PostgreSQL and its connection returned cleanly to the pool instead of pinning a management connection. - Durably retry terminal df.instances status writes (completed/failed) with a bounded budget so a transient management-plane outage cannot leave the row non-terminal while the engine execution is terminal.
80d3df0 to
2a4478d
Compare
Summary
df.start()graph rowspg_xact_status()with bounded single-shot activities plus deterministic durable backoffcontinue_as_newProblem
df.start()persistsdf.instances/df.nodesin the caller transaction but commits the duroxide start separately. The graph loader previously stopped waiting after five seconds. A legal transaction held for eight seconds then committed with the control-plane row stillpending, no SQL effect, and the engine execution terminallyFailedwithInstance <id> not found after 5s.Synthetic reproducer:
The new regression also terminates a lock-blocked management backend and holds the lock long enough for the next probe to time out, proving both transient error paths retry and the effect still occurs exactly once.
Compatibility
No extension schema, ABI, or persisted
dfdata changes. HistoricalFunctionInputpayloads defaultorigin_xidtoNoneand schedulepg_durable::activity::load-function-graphwith the same raw input bytes and operation order. New starts alone use the versioned transaction-aware probe activity. Upgrade B1 passed against every supported v0.2.2-v0.2.6 schema on PostgreSQL 17 and 18.Validation
cargo fmt -p pg_durable -- --checkcargo buildandcargo clippy -- -D warningscargo pgrx test: 333 passed, 0 failed, 16 ignored each